-
-
Notifications
You must be signed in to change notification settings - Fork 297
Use Windows API to spawn processes instead of powershell #1269
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
…f powershell (bar) re LGUG2Z#1174
whkd still remains to be spawned with powershell as switch to spawn directly will cause a console window to pop up
The previous commits handles the trailing space issue, this commit concludes the fix by handling the the termination of AHK processes with winapi. Note the added check for komorebic.ahk without the quote, as this is new behavior that can happen when the provided path has no spaces, introduced after swapping over to winapi fix LGUG2Z#1174
|
@LGUG2Z please test if |
|
If this PR is merged into whkd, we can swap whkd spawning to winapi as well, I have tested that it works however one problem is that with that change the startup seems to be too fast. I think The other problem I noticed is suppose I have config setup for my Let's hold off on merging this until I have these problems fixed. |
|
I have addressed the start up crash problem in the bar. The bar will now attempt to connect to Regarding this I will handle it in a separate PR after this PR is merged
This PR is ready for review now, but depends on whkd pr, else a console window will appear when |
|
I'm going to park this PR until the 0.1.35 dev cycle |
efeefb7 to
974e5a2
Compare
6926478 to
75a0506
Compare
| let state = serde_json::from_str::<komorebi_client::State>(&komorebi_client::send_query( | ||
| &SocketMessage::State, | ||
| )?)?; | ||
| let retry_duration = Duration::from_secs(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we can retry in a loop here - we need to bail out if something isn't working to avoid spinning cycles and provide some form of user feedback that they can share when they open a support thread
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, I don't think this is ideal. I had to make this change because the bars would now launch too fast, crashing as komorebi is yet ready to accept connections. If there is a way to know when komorebi has completed its startup and then we launch the bars that would be more ideal, but I don't know of a nice way of implementing that (simplest idea i have is using a tmp file to signal that startup is complete).
Some possible improvement I can think of
- Wait for a specified duration in
komorebicbefore launching the bars - Within the bar set a upper bound on the number of retries
But for both approaches finding the right duration/number of retries that caters to each user's machine might be tough.
What would be your approach to this problem?
Whoops, this got lost in my sea of GitHub notifications... I typically use a successful exit code for I think in this case I would prefer to see this logic kept in |
9da6485 to
3019eaf
Compare
|
Hi @Dethada, please see LGUG2Z/whkd#87 |
07b6367 to
b613986
Compare
whkd, as it will spawn a visible console window if we use win api for its process creation. I suspect this can be solved onwhkdside but I'll leave it as is for nowkomorebic stop --ahkdoesn't stop autohotkey because of trailing space in command line #1174 now that we don't have powershell adding trailing space to the commandline